Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Added blueprint.json for live preview
Updated gpl license from gpl-2 to gpl-3 #39
|
Test on Playground |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a major refactoring of the Perform plugin's settings interface, migrating from a traditional PHP-rendered settings page to a React-based single-page application. The changes update the plugin to version 1.5.0 and change the license from GPLv2 to GPLv3.
Key changes:
- React-based settings UI using WordPress components
- Centralized settings field definitions in Helpers class
- Enhanced security with nonce verification and capability checks
- Improved field sanitization logic based on field types
- Updated GitHub repository references and bug report URLs
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| perform.php | Updates version to 1.5.0 and changes license to GPLv3 |
| package.json | Adds WordPress React dependencies and updates version/license |
| webpack.config.js | Updates GitHub repository URL and removes trailing space |
| src/Includes/Helpers.php | Adds extensive settings field definitions and helper methods |
| src/Admin/Settings/Menu.php | Refactors to use React app with localized data |
| src/Admin/Actions.php | Adds WordPress components and localizes settings data |
| assets/src/js/admin/*.jsx | New React components for settings interface |
| assets/src/css/admin/settings.css | New styles for React-based settings page |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return array_map( [ __CLASS__, __METHOD__ ], $var ); | ||
| } else { | ||
| return is_scalar( $var ) ? sanitize_text_field( wp_unslash( $var ) ) : $var; | ||
| // Recursively clean array values by calling this same method. |
There was a problem hiding this comment.
[nitpick] The refactored code now uses an explicit method name 'clean' instead of METHOD. While this works correctly, the original METHOD approach was more maintainable as it would automatically update if the method is renamed. Consider adding a comment explaining why the explicit method name is preferred here, or document that method renaming should update this reference.
| // Recursively clean array values by calling this same method. | |
| // Recursively clean array values by calling this same method. | |
| // Note: The method name 'clean' is used explicitly here for recursion. | |
| // If this method is renamed, update the string 'clean' accordingly. |
| // If associative array (value=>label) check keys, otherwise check values | ||
| $keys = array_keys( $opts ); | ||
| $vals = array_values( $opts ); | ||
| if ( array_diff_key( $opts, array_values( $opts ) ) ) { |
There was a problem hiding this comment.
This logic to detect associative arrays is unclear and potentially fragile. The condition checks if the array has different keys after array_values(), but this is an indirect way to detect associative arrays. Consider using a clearer approach like checking if array_keys($opts) !== range(0, count($opts) - 1) or adding a helper function with a descriptive name like 'is_associative_array()'.
| if ( array_diff_key( $opts, array_values( $opts ) ) ) { | |
| if ( array_keys( $opts ) !== range( 0, count( $opts ) - 1 ) ) { |
| setSaving(true); | ||
| setMessage(null); | ||
| try { | ||
| const res = await fetch(ajaxurl, { |
There was a problem hiding this comment.
The code uses the global 'ajaxurl' variable without checking if it exists. While this is typically available in WordPress admin, it's better to access it safely via window.ajaxurl or provide a fallback. Consider adding a check or using window.performwpSettings to pass the ajax URL explicitly.
| const res = await fetch(ajaxurl, { | |
| const ajaxUrl = window.performwpSettings?.ajaxurl || window.ajaxurl; | |
| if (!ajaxUrl) { | |
| throw new Error('AJAX URL not defined.'); | |
| } | |
| const res = await fetch(ajaxUrl, { |
| // mutate initialValues object won't update memo, so reset by rebuild: setFieldValues equals current, but we need to reset initialValues - simplest approach: set initial snapshot to current by resetting via a state. | ||
| // We'll set the initialValues by replacing the state used for comparison: emulate by setting all initialValues to current values via a ref - but here we'll just clear dirty by resetting initialValues via resetting fieldValues baseline. | ||
| // For simplicity, update initialValues by assigning to window.performwpSettings._initial = fieldValues (not ideal), but we can update local initialValues via a small trick: setFieldValues to same and update a savedSnapshot state. | ||
| // Implement savedSnapshot state instead. |
There was a problem hiding this comment.
These commented-out thoughts/implementation notes should be removed. They describe the problem-solving process but don't add value to the final code. The implementation using savedSnapshot is clear from the code itself. Remove these comments to improve code cleanliness.
| // mutate initialValues object won't update memo, so reset by rebuild: setFieldValues equals current, but we need to reset initialValues - simplest approach: set initial snapshot to current by resetting via a state. | |
| // We'll set the initialValues by replacing the state used for comparison: emulate by setting all initialValues to current values via a ref - but here we'll just clear dirty by resetting initialValues via resetting fieldValues baseline. | |
| // For simplicity, update initialValues by assigning to window.performwpSettings._initial = fieldValues (not ideal), but we can update local initialValues via a small trick: setFieldValues to same and update a savedSnapshot state. | |
| // Implement savedSnapshot state instead. |
| } | ||
| }); | ||
|
|
||
|
|
There was a problem hiding this comment.
The file contains both the new React app initialization (lines 4-12) and what appears to be legacy code for the old form-based settings (lines 15+). If the old PHP-rendered form is no longer used, this legacy code should be removed. If both approaches coexist during a transition period, add comments explaining this temporary state.
| // TEMPORARY: The following legacy form-handling code is retained during the transition to the new React-based settings page. | |
| // Once all settings functionality has been migrated to React, this code can be safely removed. |
| justifyContent: 'flex-end', | ||
| gap: 12, | ||
| alignItems: 'center', | ||
| marginBottom: '-20px', |
There was a problem hiding this comment.
[nitpick] Using a negative margin to adjust positioning is a code smell that suggests the layout structure may need adjustment. Consider reviewing the parent container's padding/margin instead of using negative values, which can cause unexpected layout issues and make the component less reusable.
| marginBottom: '-20px', |
| $sanitized_post[ $key ] = sanitize_text_field( $raw_val ); | ||
| } | ||
| } else { | ||
| // No field definition found – fall back to a safe cleaning |
There was a problem hiding this comment.
When no field definition is found, the code falls back to sanitize_text_field for scalar values. However, this may not be appropriate for all field types (e.g., URLs, emails, numbers). Consider logging a warning when a field definition is not found, as this could indicate a mismatch between frontend and backend field definitions, potentially allowing unsanitized data if Helpers::clean() doesn't handle the specific data type properly.
| // No field definition found – fall back to a safe cleaning | |
| // No field definition found – log a warning and fall back to a safe cleaning | |
| error_log( | |
| sprintf( | |
| '[Perform] Warning: No field definition found for key "%s" with value "%s". Falling back to sanitize_text_field or Helpers::clean. This may indicate a mismatch between frontend and backend field definitions.', | |
| $key, | |
| is_scalar( $raw_val ) ? (string) $raw_val : json_encode( $raw_val ) | |
| ) | |
| ); |
| @@ -0,0 +1,35 @@ | |||
| import { Button, Spinner } from '@wordpress/components'; | |||
| import { useEffect } from '@wordpress/element'; | |||
There was a problem hiding this comment.
Unused import useEffect.
| import { useEffect } from '@wordpress/element'; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
2 participants
Changes